net/nat clean-ups by qmonnet · Pull Request #1550 · githedgehog/dataplane

qmonnet · 2026-05-20T22:08:06Z

Some Claude-assisted clean-ups for the net and nat (first commit) crates, in particular to get rid of the confusing leftovers for "unidirectional" flow keys, now that bidirectional ones are gone.

Based on top of #1548.

Copilot

Pull request overview

This PR removes legacy “unidirectional flow key” scaffolding by flattening FlowKey into a single struct and updating NAT/flow-table code to use the simplified API. It also refactors NAT error-to-DoneReason mapping to use From conversions instead of helper functions.

Changes:

Refactor net::FlowKey from an enum + FlowKeyData/Uni wrapper into a single FlowKey struct with direct accessors and TryFrom<&Packet>.
Update NAT, flow-filter, and flow-entry code/tests to construct and mutate flow keys via FlowKey::new, setters, and TryFrom<&Packet>.
Replace NAT translate_error() helpers with From<&…Error> for DoneReason implementations.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
net/src/packet/mod.rs	Updates packet flow-key refresh to use `TryFrom<&Packet>` (removes `Uni` wrapper).
net/src/packet/meta.rs	Updates flow-key rewrite when setting `src_vpcd` to use `FlowKey::new` and accessors.
net/src/lib.rs	Stops re-exporting removed `FlowKeyData`.
net/src/flows/flow_key.rs	Core refactor: removes `FlowKeyData` + enum variant, introduces single `FlowKey` struct and `TryFrom<&Packet>`.
net/src/flows/display.rs	Simplifies `Display` impl by implementing directly on `FlowKey`.
nat/src/stateless/nf.rs	Refactors error-to-`DoneReason` mapping via `From<&StatelessNatError>`.
nat/src/stateful/test.rs	Updates tests to build flow keys via `TryFrom<&Packet>` (no `Uni`).
nat/src/stateful/nf.rs	Uses `FlowKey::new`/accessors and refactors `DoneReason` mapping via `From<&StatefulNatError>`.
nat/src/stateful/flows.rs	Updates flow-key field access to new accessors.
nat/src/stateful/allocation.rs	Adds `From<&AllocatorError> for DoneReason` to centralize mapping.
nat/src/portfw/flow_state.rs	Updates port-forward flow-key construction/mutation to use `FlowKey` setters/accessors.
nat/src/icmp_handler/icmp_error_msg.rs	Adds `From<&IcmpErrorMsgError> for DoneReason` to share mapping across NAT modes.
flow-filter/src/tests.rs	Updates tests to use `TryFrom<&Packet>` flow-key creation.
flow-entry/src/flow_table/table.rs	Updates flow-table tests to construct flow keys via `FlowKey::new`.
flow-entry/src/flow_table/nf_lookup.rs	Updates flow lookup to use `TryFrom<&Packet>` and removes unused imports.

daniel-noland

Yup. This is a clear improvement

Rather than the large translate_error() functions, implement From for DoneReason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>

FlowKey is an enum with a single Unidirectional variant wrapping FlowKeyData, the second (bidirectional) variant was removed some time ago. Turn FlowKey into a tuple struct so the wrapping no longer requires pattern-matching, and drop the now-trivial match arms in PartialEq, Hash, and the Display impl. The inner field is left pub for now, a follow-up commit will merge FlowKey and FlowKeyData into a single struct. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>

FlowKey had become a tuple struct wrapping FlowKeyData, with accessors data() and data_mut() used at every call site. The two types described the same flow tuple, so we can fold FlowKeyData's fields, methods, and Hash implementation directly into FlowKey and drop the indirection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>

FlowKey used to support a bidirectional variant alongside the unidirectional one, and the builder was named uni() to disambiguate. Bidirectional keys were dropped a while ago, and the remaining builder named "uni()" is now confusing. Rename it to "new()" instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>

Uni<T> existed to tag a packet for unidirectional FlowKey extraction back when bidirectional keys were also supported. Bidirectional keys are gone, so Uni is not longer necessary, it simply adds dead weight that every call site has to thread through, and makes the code less clear. Convert the TryFrom impl to accept &Packet<Buf> directly, delete the Uni struct, and drop the wrapper at the call sites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>

coderabbitai · 2026-06-09T13:45:46Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: cf5e2967-6050-44fc-801b-e626f2ae9e53

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch pr/qmonnet/nat-clean-ups

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

qmonnet requested review from Copilot and mvachhar May 20, 2026 22:08

qmonnet self-assigned this May 20, 2026

qmonnet requested a review from a team as a code owner May 20, 2026 22:08

qmonnet added the area/nat Related to Network Address Translation (NAT) label May 20, 2026

Copilot started reviewing on behalf of qmonnet May 20, 2026 22:08 View session

Copilot AI reviewed May 20, 2026

View reviewed changes

Comment thread nat/src/stateless/nf.rs

qmonnet force-pushed the pr/qmonnet/nat-clean-ups branch from 8134920 to 01c0aa5 Compare May 20, 2026 22:21

qmonnet force-pushed the pr/qmonnet/split-static-nat branch from 41463b9 to 1bee3c4 Compare May 20, 2026 22:25

qmonnet force-pushed the pr/qmonnet/nat-clean-ups branch from 01c0aa5 to 3ec9fb9 Compare May 20, 2026 22:26

qmonnet force-pushed the pr/qmonnet/split-static-nat branch from 1bee3c4 to 4bea4c4 Compare May 21, 2026 16:44

qmonnet force-pushed the pr/qmonnet/nat-clean-ups branch from 3ec9fb9 to ee88caa Compare May 21, 2026 16:55

qmonnet force-pushed the pr/qmonnet/split-static-nat branch from 4bea4c4 to 1d0ea0e Compare May 21, 2026 17:00

qmonnet force-pushed the pr/qmonnet/nat-clean-ups branch from ee88caa to aa5f5fe Compare May 21, 2026 17:00

mvachhar requested a review from Fredi-raspall May 27, 2026 15:34

qmonnet force-pushed the pr/qmonnet/split-static-nat branch 2 times, most recently from 55f2af7 to 76c39fc Compare May 30, 2026 02:46

qmonnet force-pushed the pr/qmonnet/nat-clean-ups branch from aa5f5fe to 9485bfb Compare May 30, 2026 02:52

qmonnet force-pushed the pr/qmonnet/split-static-nat branch 2 times, most recently from bff2fdb to 0bce3d4 Compare June 1, 2026 16:00

qmonnet force-pushed the pr/qmonnet/nat-clean-ups branch from 9485bfb to f2bb72f Compare June 1, 2026 16:00

qmonnet force-pushed the pr/qmonnet/split-static-nat branch from 0bce3d4 to a36a1c4 Compare June 1, 2026 17:09

qmonnet force-pushed the pr/qmonnet/nat-clean-ups branch from f2bb72f to 3199fcf Compare June 1, 2026 17:09

daniel-noland approved these changes Jun 1, 2026

View reviewed changes

qmonnet force-pushed the pr/qmonnet/split-static-nat branch 4 times, most recently from a98c8c9 to 4813fb0 Compare June 8, 2026 20:29

qmonnet and others added 2 commits June 9, 2026 14:44

refactor(nat): Clean a little the error translation code in NAT

3b411fb

Rather than the large translate_error() functions, implement From for DoneReason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>

qmonnet and others added 3 commits June 9, 2026 14:44

qmonnet force-pushed the pr/qmonnet/nat-clean-ups branch from 3199fcf to 3319c5e Compare June 9, 2026 13:45

qmonnet mentioned this pull request Jun 9, 2026

Static NAT + Masquerading / Static NAT + Port Forwarding #1548

Merged

qmonnet merged commit 1dd7e31 into pr/qmonnet/split-static-nat Jun 9, 2026
31 checks passed

qmonnet deleted the pr/qmonnet/nat-clean-ups branch June 9, 2026 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/nat clean-ups#1550

net/nat clean-ups#1550
qmonnet merged 5 commits into
pr/qmonnet/split-static-natfrom
pr/qmonnet/nat-clean-ups

qmonnet commented May 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

daniel-noland left a comment

Uh oh!

coderabbitai Bot commented Jun 9, 2026

Review skipped

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

qmonnet commented May 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

daniel-noland left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented Jun 9, 2026

Review skipped

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants